[control-operator] finalizer blocks deletion of task until all pods are gone#830
[control-operator] finalizer blocks deletion of task until all pods are gone#830justonedev1 wants to merge 2 commits into
Conversation
knopers8
left a comment
knopers8
left a comment
There was a problem hiding this comment.
The logic seems OK, but please try to simplify the logic and reduce nesting similarly to environment's handleFinalizer. Perhaps in a separate PR, but as you prefer.
9b20acf to
ca5ca50
Compare
knopers8
left a comment
knopers8
left a comment
There was a problem hiding this comment.
Thanks for the refactoring, it does look clearer! I would still have a few suggestions and questions though.
|
|
||
| // podDeleted deletes the Task's Pod if it still exists, and reports whether it is | ||
| // fully gone yet so the caller can wait for termination before removing the finalizer. | ||
| func (r *TaskReconciler) podDeleted(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (bool, error) { |
There was a problem hiding this comment.
The name of the function is misleading, as it does not indicate that it attempts to delete the pod. Sure, the behaviour is explained better in a comment, but one can only notice it when navigating to the function implementation. Please rename to sth like "deletePod" and also name the return value in the declaration.
| func (r *TaskReconciler) podDeleted(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (bool, error) { | |
| func (r *TaskReconciler) deletePod(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (podGone bool, error) { |
However, given that you have to negate the returned value after calling podDeleted, you could even return podExists for (arguably) easier reading?
| // until POD is gone. | ||
| // Note: if you add finalizer to a task you cannot delete it unless you remove the finalizer, run: | ||
| // kubectl patch task --type=json -p='[{"op": "remove", "path": "/metadata/finalizers"}]' | ||
| func (r *TaskReconciler) handleFinalizer(ctx context.Context, t *aliecsv1alpha1.Task, log logr.Logger) (ctrl.Result, bool, error) { |
There was a problem hiding this comment.
please document what the function returns, especially what the bool means.
| } | ||
| if !controllerutil.ContainsFinalizer(t, taskFinalizer) { | ||
| return ctrl.Result{}, true, nil | ||
| } |
There was a problem hiding this comment.
Which situation could lead us to this case, i.e. scheduled for deletion, but no finalizer? Given that we ask Reconcile to return, won't it result in an infinite loop?
There was a problem hiding this comment.
It would not result in infinite loop because it can happen only when the object was finalised but not deleted, aka in a process of deletion. Basically indempotency check. From the point of view of this controller this object is done/gone and shouldn't be acted up. But Reconciliation can be triggered by k8s and this causes Reconcile loop for given object to be no-op as it should be
| } | ||
| return ctrl.Result{}, false, nil | ||
|
|
||
| log.Info("Finalizer found, starting cleanup") |
There was a problem hiding this comment.
| log.Info("Finalizer found, starting cleanup") | |
| log.Info("Deletion timestamp exists and finalizer found, starting cleanup") |
I would propose to be more clear about conditions for starting cleanup, so one is not misled into thinking that it's enough to have finalizer present to start cleanup.
There was a problem hiding this comment.
The timestamp is automatically added by k8s, not by us when there is a request to delete any resource, so in my opinion it isn't necessary to repeat it.
2 participants
can be merged, if you think that it is good enough